-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Chat bubble style api #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
b33e419 to
193e28b
Compare
193e28b to
e7a9d13
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
==========================================
+ Coverage 96.17% 96.35% +0.18%
==========================================
Files 29 30 +1
Lines 679 713 +34
Branches 114 133 +19
==========================================
+ Hits 653 687 +34
Misses 26 26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
239e20d to
286e3f0
Compare
286e3f0 to
0cc6e7a
Compare
48bd87b to
b588cbd
Compare
13f93c7 to
7fbc808
Compare
src/chat-bubble/styles.scss
Outdated
| position: absolute; | ||
| bottom: 0; | ||
| left: 0; | ||
| width: 100%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use logical properties to keep full support for right-to-left languages.
I see that in the components package we use a plugin (csstools/use-logical) in order to enforce this, but we don't have this in this package.
I suggest to
- Change this piece of code to use logical properties
- Add the same level of linting to this package —but doesn't need to be in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks. I have applied the logical properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also look into the second suggestion. To do it in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! In order to comply, width should also be replaced with inline-size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done I have updated it.
src/chat-bubble/styles.scss
Outdated
| position: absolute; | ||
| bottom: 0; | ||
| left: 0; | ||
| width: 100%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use logical properties to keep full support for right-to-left languages.
I see that in the components package we use a plugin (csstools/use-logical) in order to enforce this, but we don't have this in this package.
I suggest to
- Change this piece of code to use logical properties
- Add the same level of linting to this package —but doesn't need to be in this PR
8248ff9 to
29b5cde
Compare
| import { TestBed } from "../app/test-bed"; | ||
| import { Actions, ChatBubbleAvatarGenAI, ChatBubbleAvatarUser, ChatContainer } from "./util-components"; | ||
|
|
||
| export default function ChatBubblePage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking]: I'd suggest to implement it using the same abstraction as in the components repo: https://github.com/cloudscape-design/components/blob/main/pages/alert/permutations.page.tsx#L10 as it's more scalable and easier to modify / test new properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, we don't have PermutationsView util in this package. We can make it reusable and use it here as well in the future. For this PR, I will not be adding it so that I can get unblocked.
vite.config.unit.mjs
Outdated
| }, | ||
| load(id) { | ||
| if (id === "/virtual:environment-mock") { | ||
| // Simple stub - the actual values are mocked when testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these values take real values instead of stub?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly, Keeping them as stubs would just duplicate the config logic since these are mostly static values. Updated to use real values.
bee1bc1 to
29b5cde
Compare
| root: "./", | ||
| resolve: { | ||
| alias: { | ||
| "../internal/environment": "/virtual:environment-mock", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the file ../internal/environment not assumed to exist anyway since otherwise named imports like this would break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The src/internal/environment.d.ts makes TypeScript happy that the file will exist, and the environment.js file is actually generated using the script scripts/environment.js during build, so we actually have lib/components/internal/environment.js after build. This alias is needed only when we unit test the source code, and since the source code doesn't have the ../internal/environment JavaScript implementation.
This creates a testing setup that handles different scenarios:
Built component tests: Import from lib/components/ → uses real generated environment file
Source function tests: Import from src/ → uses virtual mock via alias
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! This makes more sense now. On the other hand, since you now have created the getEnvironmentVariables function... would it be simpler to use it in the component code and mock the function instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking] In this case why not to use .env file for the testing environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jperals The getEnvironmentVariables function is in scripts/environment.js, but scripts are not built into lib/components during the build process. This means it wouldn't be possible to use this function inside the source code, in components code. Maybe I can change the function name to make it less confusing so that it doesn't get used inside the components.
I will change it to getBuildTimeEnvironmentConstants If you have any other suggestion please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@georgylobko Thanks for the suggestion, While .env files could work for unit testing ((like .env.test), we would end up maintaining environment variables in two different places. I think keeping all environment variables in one place provides better consistency. Since we already have the configuration in environment.js and it serves both build-time generation and testing purposes, it avoids duplication.
I think, we can improve the modularity by extracting the environment configuration into a separate utility file (e.g., scripts/utils/environment-config.js) or into scripts/utils/workspace.js similar to the components repository. We can do this in a small separate PR.
6aa057e to
2c4c34c
Compare
2c4c34c to
d0eadac
Compare
| root: "./", | ||
| resolve: { | ||
| alias: { | ||
| "../internal/environment": "/virtual:environment-mock", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking] In this case why not to use .env file for the testing environment?
d0eadac to
4e51ad5
Compare
Description
This PR introduces a comprehensive style API for the ChatBubble component, enabling developers to customize the visual appearance of chat bubbles.
Vitest Configuration: Added virtual environment mocking in vite.config.unit.mjs to resolve Vitest/Vite module resolution limitations. Unlike Jest, Vitest cannot mock non-existent files - Vite's module bundler blocks imports of unresolved modules. The style functions use conditional logic based on SYSTEM environment variable, requiring vi.doMock() to test different branches. The virtual environment mock provides a resolvable module path for Vite, enabling dynamic environment mocking during tests without build dependencies.
Related links: D347810736
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.